remote: locally index list of checksums available on cloud remotes #3634

pmrowla · 2020-04-15T09:03:33Z

❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here. If the CLI API is changed, I have updated tab completion scripts.
❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

Index changes from #3600.
Related to #2147.

Docs PR: iterative/dvc.org#1163

New behavior

Checksums available on the remote are indexed (cached) locally in a sqlite3 database. The db file will be stored as .dvc/tmp/index/<filename>.idx where filename is the SHA256 digest of the remote URL.
- Only .dir checksums, and checksums for files contained inside those directories will be indexed
- Standalone files will not be indexed
On any dvc command which queries remote status (status -c/push/pull/fetch) index will be updated to include any new .dir's which are found on the remote (or are pushed successfully to the remote)

Implementation details

remote.status/remote.cache_exists
1. Validate our index by querying for .dir checksums on the remote. If a .dir checksum is in our index, but not the remote (i.e. someone else has run gc -c), clear the entire index.
2. If a .dir checksum exists on the remote, trust that directory file contents also exists on the remote (unchanged from current behavior)
3. Update index with any new directories found on the remote (i.e. dirs that someone else has push'd since we last queried remote status)
4. Query remote for standalone files (unchanged from current behavior)
pull/fetch: If any index related mismatch/error occurs (i.e. trying to download a file that is in our index but not on the remote), clear the entire index.
push: After push operation, successfully uploaded .dir checksums and file contents checksums for the directory contents are added to the index. Files uploaded during a partially failed push will not be indexed
gc -c: Re-index will be required after a gc -c operation
Changes to the index database are only committed once, after all operations on the remote have finished.

Todo

use .dir checksum existence on remote to validate/invalidate index
unit tests for index module
functional tests for new behavior
docs
benchmarks
changes from 4/17 discussion

pmrowla · 2020-04-15T09:12:27Z

dvc/remote/index.py

+    INDEX_TABLE = "remote_index"
+    INDEX_TABLE_LAYOUT = "checksum TEXT PRIMARY KEY, " "dir INTEGER NOT NULL"


If we are using a database, would it be better to have a single .dvc/tmp/index db, with appropriate tables/columns for handling multiple remotes, vs having multiple single-table .dvc/tmp/index/... dbs?

I'm not sure how common it is for users to have multiple remotes configured, but in the case where you have multiple large remotes, as it is right now we would be duplicating data across multiple (potentially large) index db files.

@pmrowla If we are talking about storing unpacked *.dir cache files, then there doesn't seem to be a point to store them for each remote, since no matter the remote if it has *.dir present we trust that it also has all cache files from within it. So maybe it is a premature optimization for later general indexes for whole remotes.

dvc/remote/local.py

pmrowla · 2020-04-17T08:47:17Z

Some benchmarks from my machine (dual-core i7-5557U @ 3.10GHz, osx catalina):

S3 remote, 2M imagenet files, w/existing state database, but no pulled/fetched files

latest PR branch, with no existing index file (equivalent to first time running status -c or forced re-index via --clear-index)

time dvc status -c
517.33s user 103.87s system 81% cpu 12:46.00 total

latest PR branch, with existing index file

time dvc status -c
473.61s user 80.07s system 90% cpu 10:12.18 total

latest master

time dvc status -c
478.89s user 83.17s system 86% cpu 10:49.67 total

0.91.0 default (pre-optimizations)

todo

0.91.0 no-traverse = False

time dvc status -c
802.44s user 87.22s system 42% cpu 35:04.45 total

rclone (w/sanitized remote url)

time rclone sync --dry-run remote:remote/ .dvc/cache/
141.96s user 21.53s system 64% cpu 4:11.82 total

The traverse improvements from 0.91.0 to now should be more drastic on machines with more cores.
Also, it would be ideal if we had a convenient way to only benchmark the cloud/remote steps to see how we compare to tools like rclone

efiop · 2020-04-17T09:13:36Z

@pmrowla And what about the case when we add 1(or just n << N) new file to the dataset? I think it should also shine there, compared to what we had before index.

pared

Just some unnecessary fixture includes

tests/func/remote/test_index.py

pmrowla · 2020-04-17T11:34:48Z

Had discussion with @efiop, a couple of changes needed before this can be merged.

Keep behavior from current master regarding trusting .dir files on the remote
For push, only update index when entire dir has been uploaded successfully

Will also check cprofile results and update benchmarks

pmrowla · 2020-04-20T06:19:04Z

Previous benchmarks post has been updated with current master and PR branch numbers for status -c. Whenever there is no pre-existing index db (or when --clear-index is used to force reindexing), adding the initial 2M checksums to the db does add ~2 minutes of overhead, but after the index exists, the overhead for checking the existing index is negligible.

Running with cProfile, pathinfo handling code is a large bottleneck for us when checking cloud status, particularly in dvc/stage/base.py::_collect_used_dir_cache()

from python -m cProfile -o status.prof -m dvc status -c with an existing index db:

zip w/cprofile output:
cprofile.zip

pmrowla · 2020-04-20T08:15:58Z

Benchmarks for status -c after adding 1 file to an existing pushed (and indexed) dir with 100k files.

S3 remote, 100k total files in remote cache

latest PR branch

time dvc status -c
13.58s user 5.91s system 82% cpu 23.671 total

latest master

time dvc status -c
41.61s user 8.32s system 84% cpu 59.136 total

0.91.0 default

time dvc status -c
423.96s user 26.00s system 37% cpu 19:53.69 total

0.91.0 no_traverse = False

time dvc status -c
33.46s user 7.08s system 36% cpu 1:49.62 total

rclone

time rclone sync --dry-run .dvc/cache s3://...
103.33s user 67.31s system 7% cpu 36:59.68 total

dvc/remote/base.py

shcheklein · 2020-04-20T16:31:58Z

@pmrowla amazing stuff 👏 - ~7 times faster than rclone.

I'm curious where do we spend 13seconds though in this scenario 🤔

shcheklein · 2020-04-20T16:32:39Z

@pmrowla is it pathinfo handling code is a large bottleneck for us when checking cloud status, particularly in dvc/stage/base.py::_collect_used_dir_cache()?

shcheklein · 2020-04-20T16:40:16Z

Also, out of curiosity - do we actually need --drop-index option? A bit worried that we provide it to every data management related command.

dvc/command/data_sync.py

efiop · 2020-04-20T20:47:04Z

Also, out of curiosity - do we actually need --drop-index option? A bit worried that we provide it to every data management related command.

Thinking about it, looks like current --clear-index simply drops the index for specific remote. So maybe it would be more logical do create dvc remote clear-index(maybe there is a better name) instead. There doesn't seem to be a good use case from the user point of view, but it is really helpful for us when debugging or providing support. That being said, we could always ask user to rm -rf .dvc/tmp/index instead. So looks like it is indeed not strictly necessary but handy.

Looks like we could indeed drop it for now until someone asks. I think it was me who asked for this flag awhile ago, but it no longer looks like a great idea to me, I think i was thinking about something like --ignore-index originally, but I don't see a good use case for that neither.

- do not index files on partial push/upload

- skip unnecessary index check for dir contents if .dir file exists on the remote

pmrowla · 2020-04-21T03:27:59Z

--clear-index has been removed and the associated docs PR is closed

dvc/remote/base.py

dvc/remote/index.py

efiop

🔥

pmrowla added the performance improvement over resource / time consuming tasks label Apr 15, 2020

pmrowla self-assigned this Apr 15, 2020

pmrowla force-pushed the remote-sqlite-index branch from 6a74efd to b1314bd Compare April 15, 2020 09:04

pmrowla commented Apr 15, 2020

View reviewed changes

pmrowla force-pushed the remote-sqlite-index branch 2 times, most recently from 2bf0e56 to 6e6c450 Compare April 16, 2020 08:07

pmrowla mentioned this pull request Apr 17, 2020

[WIP] remote: document --clear-index iterative/dvc.org#1163

Closed

pmrowla commented Apr 17, 2020

View reviewed changes

dvc/remote/local.py Outdated Show resolved Hide resolved

pmrowla marked this pull request as ready for review April 17, 2020 06:50

pmrowla changed the title ~~[WIP] remote: locally index list of checksums available on cloud remotes~~ remote: locally index list of checksums available on cloud remotes Apr 17, 2020

pmrowla requested review from efiop, pared and skshetry April 17, 2020 06:51

pared approved these changes Apr 17, 2020

View reviewed changes

tests/func/remote/test_index.py Outdated Show resolved Hide resolved

tests/func/remote/test_index.py Outdated Show resolved Hide resolved

tests/func/remote/test_index.py Outdated Show resolved Hide resolved

efiop changed the title ~~remote: locally index list of checksums available on cloud remotes~~ [WIP] remote: locally index list of checksums available on cloud remotes Apr 17, 2020

weekly-digest bot mentioned this pull request Apr 19, 2020

Weekly Digest (12 April, 2020 - 19 April, 2020) #3651

Closed

pmrowla force-pushed the remote-sqlite-index branch from cd0c23e to 83cd0f2 Compare April 20, 2020 04:14

efiop reviewed Apr 20, 2020

View reviewed changes

dvc/remote/base.py Outdated Show resolved Hide resolved

pmrowla changed the title ~~[WIP] remote: locally index list of checksums available on cloud remotes~~ remote: locally index list of checksums available on cloud remotes Apr 20, 2020

efiop reviewed Apr 20, 2020

View reviewed changes

dvc/command/data_sync.py Outdated Show resolved Hide resolved

pmrowla added 15 commits April 21, 2020 12:07

remote: add index.intersection()

26bedbe

remote: use index when making assumptions about remote .dir contents

4251d88

add missing func tests

2674eaf

do not index standalone files

aef4973

cleanup index test fixture

b31034b

fix deepsource warnings

0d5b3f4

update autocompletion scripts/cli help message for --clear-index

7ba6305

tests: remove unused mocker

ed77f9b

push: only index successfully pushed dirs

6f8745a

- do not index files on partial push/upload

remote: revert behavior to trust .dir files on remote

9102838

- skip unnecessary index check for dir contents if .dir file exists on the remote

fix missing RemoteIndexNoop functions

dd845e0

separate index validation from status dir exists query

3e0d258

bugfix: include indexed checksums for all cache_exists cases

739202b

review fix: cleanup debug messages

4a7d5a0

tests: fix DS warning

f6a684a

pmrowla force-pushed the remote-sqlite-index branch from e6d48d6 to fc6efed Compare April 21, 2020 03:14

remove --clear-index

63c7d9d

pmrowla force-pushed the remote-sqlite-index branch from fc6efed to 63c7d9d Compare April 21, 2020 03:17

efiop reviewed Apr 21, 2020

View reviewed changes

dvc/remote/base.py Show resolved Hide resolved

efiop reviewed Apr 21, 2020

View reviewed changes

dvc/remote/index.py Outdated Show resolved Hide resolved

remove unused index functions

3ec83f6

efiop approved these changes Apr 21, 2020

View reviewed changes

efiop merged commit b04964f into iterative:master Apr 21, 2020

efiop mentioned this pull request Apr 21, 2020

remote: loading dirs is exteremely slow because of PathInfo #3635

Closed

pmrowla deleted the remote-sqlite-index branch April 22, 2020 04:37

pmrowla mentioned this pull request Apr 24, 2020

remote: use string paths over PathInfo for performance reasons #3672

Merged

3 tasks

weekly-digest bot mentioned this pull request Apr 26, 2020

Weekly Digest (19 April, 2020 - 26 April, 2020) #3683

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remote: locally index list of checksums available on cloud remotes #3634

remote: locally index list of checksums available on cloud remotes #3634

pmrowla commented Apr 15, 2020 •

edited

Loading

pmrowla Apr 15, 2020 •

edited

Loading

efiop Apr 17, 2020

pmrowla commented Apr 17, 2020 •

edited

Loading

efiop commented Apr 17, 2020 •

edited

Loading

pared left a comment

pmrowla commented Apr 17, 2020 •

edited

Loading

pmrowla commented Apr 20, 2020

pmrowla commented Apr 20, 2020 •

edited

Loading

shcheklein commented Apr 20, 2020

shcheklein commented Apr 20, 2020

shcheklein commented Apr 20, 2020

efiop commented Apr 20, 2020 •

edited

Loading

pmrowla commented Apr 21, 2020

efiop left a comment

		INDEX_TABLE = "remote_index"
		INDEX_TABLE_LAYOUT = "checksum TEXT PRIMARY KEY, " "dir INTEGER NOT NULL"

remote: locally index list of checksums available on cloud remotes #3634

remote: locally index list of checksums available on cloud remotes #3634

Conversation

pmrowla commented Apr 15, 2020 • edited Loading

New behavior

Implementation details

Todo

pmrowla Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

efiop Apr 17, 2020

Choose a reason for hiding this comment

pmrowla commented Apr 17, 2020 • edited Loading

efiop commented Apr 17, 2020 • edited Loading

pared left a comment

Choose a reason for hiding this comment

pmrowla commented Apr 17, 2020 • edited Loading

pmrowla commented Apr 20, 2020

pmrowla commented Apr 20, 2020 • edited Loading

shcheklein commented Apr 20, 2020

shcheklein commented Apr 20, 2020

shcheklein commented Apr 20, 2020

efiop commented Apr 20, 2020 • edited Loading

pmrowla commented Apr 21, 2020

efiop left a comment

Choose a reason for hiding this comment

pmrowla commented Apr 15, 2020 •

edited

Loading

pmrowla Apr 15, 2020 •

edited

Loading

pmrowla commented Apr 17, 2020 •

edited

Loading

efiop commented Apr 17, 2020 •

edited

Loading

pmrowla commented Apr 17, 2020 •

edited

Loading

pmrowla commented Apr 20, 2020 •

edited

Loading

efiop commented Apr 20, 2020 •

edited

Loading